Skip to content

Definitions: Only emit links for canonical type mentions#240

Closed
pavgust wants to merge 2 commits into
github:rc/1.18from
pavgust:imp/canonical-type-mentions
Closed

Definitions: Only emit links for canonical type mentions#240
pavgust wants to merge 2 commits into
github:rc/1.18from
pavgust:imp/canonical-type-mentions

Conversation

@pavgust

@pavgust pavgust commented Sep 27, 2018

Copy link
Copy Markdown
Contributor

In 1.17, we could emit many logically indistinguishable type mentions for the same type/location where a file was processed in many different contexts. On very large projects, this can lead to an inflation in the number of definitions links that are emitted.

With this change, we'll pick a canonical type mention for each location/type pair, and only emit link information for that.

@pavgust pavgust added this to the 1.18 milestone Sep 27, 2018
aibaars
aibaars reviewed Sep 27, 2018
View reviewed changes
Comment thread cpp/ql/src/definitions.qll Outdated
kind = "T" and
TypeMentions::isCanonical(e) and
e.(TypeMention).getMentionedType() = result and
not result instanceof ClassTemplateInstantiation and

@aibaars aibaars Sep 27, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line seems unrelated.

@pavgust pavgust Sep 27, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. It's morally part of #223, but I had somehow neglected to stage it. @jbj, for info -- let's keep this hotfix patch together, as it's what I tested.

@jbj

jbj commented Sep 27, 2018

Copy link
Copy Markdown
Contributor

Test failure in ql/cpp/ql/test/query-tests/definitions/definitions.qlref due to the suppression of ClassTemplateInstantiation:

11:47:29 Diff:
11:47:29 --- expected
11:47:29 +++ actual
11:47:29 @@ -17,7 +17,6 @@
11:47:29  | class.cpp:16:16:16:22 | mention of myClass | class.cpp:2:7:2:13 | myClass | T |
11:47:29  | class.cpp:26:29:26:32 | _val | class.cpp:26:17:26:20 | _val | V |
11:47:29  | class.cpp:35:1:35:14 | mention of myIntContainer | class.cpp:9:7:9:20 | myIntContainer | T |
11:47:29 -| class.cpp:36:1:36:12 | mention of myTContainer<char> | class.cpp:23:7:23:18 | myTContainer<char> | T |
11:47:29  | class.cpp:38:30:38:36 | mention of myClass | class.cpp:2:7:2:13 | myClass | T |
11:47:29  | class.cpp:45:2:45:8 | mention of myClass | class.cpp:2:7:2:13 | myClass | T |
11:47:29  | class.cpp:51:2:51:8 | mention of myClass | class.cpp:2:7:2:13 | myClass | T |
11:47:29 @@ -88,7 +87,6 @@
11:47:29  | type_mentions.cpp:18:8:18:15 | mention of MyClass2<1> | type_mentions.cpp:14:7:14:14 | MyClass2<1> | T |
11:47:29  | type_mentions.cpp:18:24:18:31 | mention of MyClass1 | type_mentions.cpp:1:7:1:14 | MyClass1 | T |
11:47:29  | type_mentions.cpp:27:3:27:11 | mention of MyStruct1 | type_mentions.cpp:21:10:21:18 | MyStruct1 | T |
11:47:29 -| type_mentions.cpp:35:1:35:14 | mention of BinaryTemplate<MyClass1, MyStruct1> | type_mentions.cpp:31:7:31:20 | BinaryTemplate<MyClass1, MyStruct1> | T |
11:47:29  | type_mentions.cpp:35:16:35:23 | mention of MyClass1 | type_mentions.cpp:1:7:1:14 | MyClass1 | T |
11:47:29  | type_mentions.cpp:35:40:35:48 | mention of MyStruct1 | type_mentions.cpp:21:10:21:18 | MyStruct1 | T |
11:47:29  | type_mentions.cpp:39:24:39:32 | mention of MyStruct1 | type_mentions.cpp:21:10:21:18 | MyStruct1 | T |

@pavgust, these look like valid rows that are removed and are not replaced by anything else.

@jbj jbj added the C++ label Sep 27, 2018
@pavgust

pavgust commented Sep 27, 2018

Copy link
Copy Markdown
Contributor Author

As discussed, we'll handle this differently.

@pavgust pavgust closed this Sep 27, 2018
smowton added a commit to smowton/codeql that referenced this pull request Feb 7, 2022
@smowton
Merge pull request github#240 from github/smowton/admin/extractor-for…
34add23 
…matting-todos

TRAP formatting: adopt Java's standards
MathiasVP pushed a commit to MathiasVP/ql that referenced this pull request Aug 10, 2025
@dilanbhalla
Merge pull request github#240 from microsoft/auto/sync-main-pr
8e09d96 
Sync Main (autogenerated)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@aibaars aibaars aibaars left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

C++

Projects

None yet

Milestone

1.18

Development

Successfully merging this pull request may close these issues.

3 participants

@pavgust @jbj @aibaars